Fix potentially high CPU usage on Windows by antkmsft · Pull Request #4448 · Azure/azure-sdk-for-cpp

antkmsft · 2023-03-15T02:59:27Z

This is a proposal to fix the underlying issue in #4443.
Instead of caching locale, I propose to have our own set of functions.
Closes #4443.

LarryOsterman · 2023-03-16T18:30:46Z

Now that I've had a few days to think about it, I don't really have a problem with it (especially given the test to verify that the current functions match the CRT behavior).

…into locale

RickWinter

Is the perf better than a single cached reference to locale (which is immutable)?

antkmsft · 2023-03-20T19:34:57Z

@RickWinter, it appears to be 64x faster than the caching implementation:

Inline functions for the StringExtensions class:

static std::locale const& GetLocale()
{
    static auto const locale = std::locale::classic();
    return locale;
}

static bool IsDigit2(char c) noexcept { return std::isdigit(c, GetLocale()); }
static bool IsHexDigit2(char c) noexcept { return std::isxdigit(c, GetLocale()); }
static bool IsAlphaNumeric2(char c) noexcept { return std::isalnum(c, GetLocale()); }
static bool IsSpace2(char c) noexcept { return std::isspace(c, GetLocale()); }
static bool IsPrintable2(char c) noexcept { return std::isprint(c, GetLocale()); }

Test:

using Azure::Core::_internal::StringExtensions;
auto const iterations = 10000;

auto start = std::chrono::system_clock::now();
for (auto n = 0; n < iterations; ++n)
{
for (unsigned i = 0; i <= 255; ++i)
{
    auto const c = static_cast<char>(static_cast<unsigned char>(i));
    static_cast<void>(StringExtensions::IsDigit(c));
    static_cast<void>(StringExtensions::IsHexDigit(c));
    static_cast<void>(StringExtensions::IsAlphaNumeric(c));
    static_cast<void>(StringExtensions::IsSpace(c));
    static_cast<void>(StringExtensions::IsPrintable(c));
}
}

auto elapsed = std::chrono::system_clock::now() - start;
std::cout << "-------------\n\nHandwritten: "
        << static_cast<double>(
                std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count()) << "ms\n";

start = std::chrono::system_clock::now();
for (auto n = 0; n < iterations; ++n)
{
for (unsigned i = 0; i <= 255; ++i)
{
    auto const c = static_cast<char>(static_cast<unsigned char>(i));
    static_cast<void>(StringExtensions::IsDigit2(c));
    static_cast<void>(StringExtensions::IsHexDigit2(c));
    static_cast<void>(StringExtensions::IsAlphaNumeric2(c));
    static_cast<void>(StringExtensions::IsSpace2(c));
    static_cast<void>(StringExtensions::IsPrintable2(c));
}
}

elapsed = std::chrono::system_clock::now() - start;
std::cout << "\nStd-cached : "
        << static_cast<double>(
                std::chrono::duration_cast<std::chrono::milliseconds>(elapsed).count())
        << "ms\n\n-------------\n";

Results:

-------------

Handwritten: 34ms

Std-cached : 2181ms

-------------

antkmsft · 2023-03-20T19:54:46Z

And:

56x faster than std:: version that does no caching
6.8x faster than the std:: version that takes no locale (we wouldn't have used it because it is machine dependent, but good for comparison)

sdk/core/azure-core/CHANGELOG.md

sdk/core/azure-core/inc/azure/core/internal/strings.hpp

ahsonkhan · 2023-03-21T20:55:54Z

sdk/identity/azure-identity/CHANGELOG.md

+- [[#4443]](https://github.com/Azure/azure-sdk-for-cpp/issues/4443) Fixed potentially high CPU usage on Windows.
+


I don't think it is necessary to call this out in the identity changelog. The use-case here wouldn't impact the customer perf, since it was only happening in the error path of ThrowIfNotSafeCmdLineInput. That isn't a case where there would be a scalability issue causing high CPU usage.

I was thinking about that, but the change needs to be described. The customers want to know what they are getting when they are upgrading. Potentially, this still may lead to higher CPU usage, so I see no better description.

And it is not in the error path, it is a part of normal flow - when creating credential and during each call of GetToken().

I get that GetToken() might be transitively impacted (by implementation detail changes in core), but the only change that impacted identity, is the following which is in the error path:
ThrowIfNotSafeCmdLineInput

sdk/core/azure-core/test/ut/string_test.cpp

* Add GetCredentialName() (#4428) * Add GetCredentialName() * Update * Undo accidental change * Clang-format * Call GetCredentialName() instead of using constant; Return in-place constructed name; Explicit tests for GetCredentialName() * PR feedback * constructor parameter + non-virtual GetCredentialName() * Update sdk/core/azure-core/CMakeLists.txt * Update sdk/identity/azure-identity/test/ut/client_secret_credential_test.cpp * Update sdk/identity/azure-identity/test/ut/client_certificate_credential_test.cpp * GCC and Clang warnings * Promote ThrowIfNotSafeCmdLineInput() to private member; avoid copies when calling GetCredentialName() * Spelling * Fix deprecated usage * Fix iteration * Clang-format --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Sync eng/common directory with azure-sdk-tools for PR 5702 (#4453) * add healthinsights to Test-SampleMetadata script * spacing * update productSlug to azure-health-insights --------- Co-authored-by: Asaf Levi <asaflevi@microsoft.com> * Use aka.ms link to Identity troubleshooting (#4449) * Use aka.ms link to Identity troubleshooting * Update default_azure_credential.cpp * Update default_azure_credential.cpp --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Undocument ChainedCred usage by DefaultAzCred & remove friend and private ctor (#4447) * Undocument ChainedCred usage by DefaultAzCred & remove friend and private ctor * Clang warning fix --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Sync eng/common directory with azure-sdk-tools for PR 5691 (#4450) * typespec renaming * add back scripts for cadl * support .cadl and .tsp * rename * add newline at the end of file --------- Co-authored-by: FAREAST\chunyu <chunyu@microsoft.com> * fix cspell for readme.ms in libcurl sterss test (#4441) * fix cspell * capitalize Valgrind and ubuntu * sdada * fix2 * Simpler identity logging (#4455) * Simpler identity logging * Even simpler * Remove refactoring artifact * Cosmetic change * foreach --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * 3rd Party Libs in Samples (#4408) Adding guidance on the proper usage of 3rd Party Libraries in our samples. * Update changelog with issue link (#4458) Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Tests: replace most `EXPECT_TRUE(a OP b)` with `EXPECT_OP(a, b)` (#4457) * Tests: replace most `EXPECT_TRUE(a OP b)` with `EXPECT_OP(a, b)` * Undo unnecessary change --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Revert "[check-spelling] Temporarily pin Node 18 to 18.13.0 (#5537)" (#4456) This reverts commit 8a02e02adfc0d213509fce2764132afa74bd4ba4. Co-authored-by: Mike Harder <mharder@microsoft.com> * Fix potentially high CPU usage on Windows (#4448) * Fix potentially high CPU usage on Windows * Undo unnecessary formatting * Undo unnecessary changelog * Undo unnecessary formatting * Undo unnecessary formatting * Uninclude locale * Add issue link to changelog * EXPECT_TRUE(a == b) => EXPECT_EQ(a, b) * Update second changelog with link as well --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Ensure the comparison is unsigned to unsigned (#4464) * Ensure the comparison is unsigned to unsigned * Remove cast * Sync eng/common directory with azure-sdk-tools for PR 5742 (#4465) * add some default output to see about minimizing any occurrence of the task failing for no reason. perhaps having some output will allow devops to have an easier time with the invocation * update message * Update eng/common/scripts/trust-proxy-certificate.ps1 Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> * Update CODEOWNERS (#4461) Fixup an invalid user * Organize applying Identity log prefix (#4459) * Organize applying Identity log prefix * logLevel * Cosmetic changes --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> * Explicitly set PSNativeCommandArgumentPassing to Legacy for git push script (#4481) https://learn.microsoft.com/en-us/powershell/scripting/learn/experimental-features?view=powershell-7.3#psnativecommandargumentpassing Do to that breaking change in PS 7.3 we need to opt into the legacy arg parsing. Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> * Fix unmatched parenthesis in doc (#4482) * fix concurrent upload failures (#4484) * Sync eng/common directory with azure-sdk-tools for PR 5726 (#4488) * rerun flag * rerun failed stress test * naming & commenting * update * function and var renaming for better readability * readability & exit on error --------- Co-authored-by: Albert Cheng <albertcheng@microsoft.com> --------- Co-authored-by: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com> Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com> Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com> Co-authored-by: Asaf Levi <asaflevi@microsoft.com> Co-authored-by: FAREAST\chunyu <chunyu@microsoft.com> Co-authored-by: George Arama <50641385+gearama@users.noreply.github.com> Co-authored-by: Ronnie Geraghty <ronniegerag@gmail.com> Co-authored-by: Mike Harder <mharder@microsoft.com> Co-authored-by: Rick Winter <rick.winter@microsoft.com> Co-authored-by: Wes Haggard <weshaggard@users.noreply.github.com> Co-authored-by: Wes Haggard <Wes.Haggard@microsoft.com> Co-authored-by: JinmingHu <jinmhu@microsoft.com> Co-authored-by: Albert Cheng <albertcheng@microsoft.com>

* Fix potentially high CPU usage on Windows * Undo unnecessary formatting * Undo unnecessary changelog * Undo unnecessary formatting * Undo unnecessary formatting * Uninclude locale * Add issue link to changelog * EXPECT_TRUE(a == b) => EXPECT_EQ(a, b) * Update second changelog with link as well --------- Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>

antkmsft added 5 commits March 14, 2023 19:52

Fix potentially high CPU usage on Windows

5af287e

Undo unnecessary formatting

4ece171

Undo unnecessary changelog

a7e3d32

Undo unnecessary formatting

ffbad6b

Undo unnecessary formatting

4def816

antkmsft added the Azure.Core label Mar 15, 2023

antkmsft requested a review from LarryOsterman March 15, 2023 04:13

Uninclude locale

c64ed7e

antkmsft marked this pull request as ready for review March 16, 2023 18:47

antkmsft requested review from RickWinter, ahmadmsft, ahsonkhan, anilba06, gearama, gkostal, olkroshk, schaabs and vhvb1989 as code owners March 16, 2023 18:47

antkmsft and others added 5 commits March 16, 2023 12:46

Add issue link to changelog

6abbea8

EXPECT_TRUE(a == b) => EXPECT_EQ(a, b)

8f6375f

Merge branch 'main' into locale

3805784

Update second changelog with link as well

8b9c9ea

Merge branch 'locale' of https://github.com/antkmsft/azure-sdk-for-cpp …

0828d49

…into locale

RickWinter approved these changes Mar 20, 2023

View reviewed changes

antkmsft merged commit ba08657 into Azure:main Mar 20, 2023

antkmsft deleted the locale branch March 20, 2023 20:06

ahsonkhan reviewed Mar 21, 2023

View reviewed changes

sdk/core/azure-core/CHANGELOG.md Show resolved Hide resolved

ahsonkhan reviewed Mar 21, 2023

View reviewed changes

sdk/core/azure-core/inc/azure/core/internal/strings.hpp Show resolved Hide resolved

ahsonkhan reviewed Mar 21, 2023

View reviewed changes

sdk/core/azure-core/inc/azure/core/internal/strings.hpp Show resolved Hide resolved

ahsonkhan reviewed Mar 21, 2023

View reviewed changes

sdk/core/azure-core/test/ut/string_test.cpp Show resolved Hide resolved

LarryOsterman mentioned this pull request Mar 30, 2023

Merge main into feature/amqp. (#4491) #4497

Closed

14 tasks

ahsonkhan mentioned this pull request Apr 6, 2023

Core April GA Release #4515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potentially high CPU usage on Windows#4448

Fix potentially high CPU usage on Windows#4448
antkmsft merged 11 commits intoAzure:mainfrom
antkmsft:locale

antkmsft commented Mar 15, 2023

Uh oh!

LarryOsterman commented Mar 16, 2023

Uh oh!

RickWinter left a comment •

edited

Loading

Uh oh!

antkmsft commented Mar 20, 2023

Uh oh!

antkmsft commented Mar 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan Mar 21, 2023

Uh oh!

antkmsft Mar 22, 2023

Uh oh!

antkmsft Mar 22, 2023

Uh oh!

ahsonkhan Mar 27, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		- [[#4443]](https://github.com/Azure/azure-sdk-for-cpp/issues/4443) Fixed potentially high CPU usage on Windows.

Conversation

antkmsft commented Mar 15, 2023

Uh oh!

LarryOsterman commented Mar 16, 2023

Uh oh!

RickWinter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antkmsft commented Mar 20, 2023

Uh oh!

antkmsft commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan Mar 21, 2023

Choose a reason for hiding this comment

Uh oh!

antkmsft Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

antkmsft Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RickWinter left a comment •

edited

Loading

antkmsft commented Mar 20, 2023 •

edited

Loading